(preact-iso): Use pushState/ replaceState for useLocation().route method by piotr-cz · Pull Request #413 · preactjs/wmr

piotr-cz · 2021-03-09T11:04:56Z

At this moment when using useLocation().route('/foobar') window location doesn't change: neither history.pushState nor history.replaceState method is used.

This pull request

changes method behavior in that window location will be added to history via pushState.
allows using object as a parameter so it's possible to use replaceState

// Use history.pushState
route('/foobar);
route({ url: '/foobar' });
route({ url: '/foobar', replace: false });

// Use history.replaceState
route({ url: '/foobar', replace: true });

Originally the useReducer hook dispatcher had a third push argument, but dispatchers consume only two arguments.

wmr/packages/preact-iso/router.js

Line 4 in 58f1bff

const UPDATE = (state, url, push) => {

I'm not sure if I should bump patch or minor version with changeset?

changeset-bot · 2021-03-09T11:04:58Z

🦋 Changeset detected

Latest commit: 3d0d9f4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
preact-iso	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

developit

It looks like this was already supported?

route('/url', true)

(admittedly, push=true might be a better default)

developit · 2021-03-09T16:39:43Z

packages/preact-iso/router.d.ts

 	path: string;
 	query: Record<string, string>;
-	route: (url: string) => void;
+	route: (url: string | { url: string, replace?: boolean }) => void;


Wondering if we can use (url, replace) here? That would match preact-router.

piotr-cz · 2021-03-09T18:04:18Z

@developit
Unfortunately passing replace flag as additional argument doesn't work, as route is actually a reducers dispatcher, which accepts only one argument (see docs > useReducer):

wmr/packages/preact-iso/router.js

Lines 44 to 51 in 58f1bff

    
           const [url, route] = useReducer(UPDATE, location.pathname + location.search); 
        
           const value = useMemo(() => { 
        
           	const u = new URL(url, location.origin); 
        
           	const path = u.pathname.replace(/(.)\/$/g, '$1'); 
        
           	// @ts-ignore-next 
        
           	return { url, path, query: Object.fromEntries(u.searchParams), route }; 
        
           }, [url]);

Probably we would have to refactor router code to not use reducer in this case (which may be not a bad idea to handle history outside/ without reducer).

piotr-cz · 2021-03-11T09:29:06Z

I prefer route(url, replace) as well, but as long as there is no better solution, let's keep this PR opened for reference

piotr-cz · 2021-03-12T09:37:29Z

I've created new PR in #424 to fix the bug with location change after using useLocation().route(path).

After it's resolved it will be easier to move forward with adding an option such as route(path, replace) either with this PR new one.

developit · 2021-03-14T19:16:10Z

Nice - thanks for pulling that out, just merged it.

packages/preact-iso/router.js

developit · 2021-03-14T19:26:51Z

packages/preact-iso/router.js

+	} else if (url instanceof PopStateEvent) {
 		url = location.pathname + location.search;
+	// manual invocation: route({ path, replace })
+	} else if (typeof url === 'object') {


Here's what I'd suggest, in order to preserve the duck typing (as mentioned in previous comment):

const UPDATE = (state, update) => { /** @type {boolean|undefined} - History state update strategy */ let push, url; if (!update || typeof update === 'string') { url = update; push = true; } else if (update.type === 'click') { // user click // @ts-ignore-next const link = update.target.closest('a[href]'); if (!link || link.origin != location.origin) return state; update.preventDefault(); url = link.pathname + link.search + link.hash; push = true; } else if (update.type === 'popstate') { // navigation url = location.pathname + location.search + location.hash; } else { // manual invocation: route(url) or route({ url, replace }) url = update.url || update; push = !url.replace; } if (push === true) history.pushState(null, '', url); else if (push === false) history.replaceState(null, '', url); return url; }

Should we allow passing a value that evaluates to false (undefined/ null) with route method?
If so, then should it be added to history?

if (!update || typeof update === 'string') { // ^

When the update is not either string or event I'd say it must be an object?

else { // manual invocation: route(url) or route({ url, replace }) url = update.url || update; // ^

I've updated PR with code you suggested, adapting to eslint/ prettier rules

@developit
What do you think about my comments above?

# Conflicts: # packages/preact-iso/router.js

piotr-cz · 2021-03-15T10:11:45Z

I've updated PR to use code suggested in #413 (comment)

However I'm not happy with the route({ url, replace }) signature (proposed by me) and would prefer route(url, replace) instead.
Let's not merge this PR yet, we can do better.

developit · 2021-03-19T17:07:49Z

@piotr-cz I think we might be able to create a wrapper function in the provider that swaps the argument signature:

-	const [url, route] = useReducer(UPDATE, location.pathname + location.search);
+	const [url, doRoute] = useReducer(UPDATE, location.pathname + location.search);
+	const route = useCallback((url, replace) => doRoute({ url, replace }), []);

It might even be possible to simplify the history event handling this way too.

piotr-cz · 2021-03-22T09:07:16Z

@developit
Updated, thank you for suggestion!

piotr-cz · 2021-03-24T09:11:51Z

Just one question:
Should I add changeset with patch or minor version bump?
As the route method is not documented, I guess patch is enough.

JoviDeCroock · 2021-03-24T09:15:43Z

I'd go for patch yea

Use history.replaceState for route function

56f83c8

piotr-cz added 2 commits March 9, 2021 12:12

Add semicolons

43214d3

Add option to use object as a path

0faacea

piotr-cz changed the title ~~(preact-iso): Use history.replaceState for route function~~ (preact-iso): Use pushState/ replaceState for useLocation().route method Mar 9, 2021

Change object signature

25fd059

developit reviewed Mar 9, 2021

View reviewed changes

Merge branch 'main' into feature/preact-iso-route-location

7337f1c

developit mentioned this pull request Mar 14, 2021

(preact-iso): Router: Fix change location after useLocation().route() #424

Merged

developit reviewed Mar 14, 2021

View reviewed changes

piotr-cz added 2 commits March 15, 2021 10:47

Merge branch 'main' into feature/preact-iso-route-location

1f5654b

# Conflicts: # packages/preact-iso/router.js

Update code as per preactjs#413 (comment)

d772def

Wrap reducer to allow different signature

81f18f7

Add changeset

3d0d9f4

developit mentioned this pull request Mar 30, 2021

[preact-iso] Router: reset scroll position when navigating forwards #496

Merged

Uh oh!

Conversation

piotr-cz commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

developit left a comment

Choose a reason for hiding this comment

Uh oh!

developit Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

piotr-cz commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piotr-cz commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piotr-cz commented Mar 12, 2021

Uh oh!

developit commented Mar 14, 2021

Uh oh!

Uh oh!

developit Mar 14, 2021

Choose a reason for hiding this comment

Uh oh!

piotr-cz Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

piotr-cz Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

piotr-cz Mar 22, 2021

Choose a reason for hiding this comment

Uh oh!

piotr-cz commented Mar 15, 2021

Uh oh!

developit commented Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piotr-cz commented Mar 22, 2021

Uh oh!

piotr-cz commented Mar 24, 2021

Uh oh!

JoviDeCroock commented Mar 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

piotr-cz commented Mar 9, 2021 •

edited

Loading

changeset-bot bot commented Mar 9, 2021 •

edited

Loading

piotr-cz commented Mar 9, 2021 •

edited

Loading

piotr-cz commented Mar 11, 2021 •

edited

Loading

piotr-cz Mar 15, 2021 •

edited

Loading

developit commented Mar 19, 2021 •

edited

Loading